Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base zoom speed on browser refresh rate #12211

Closed
wants to merge 10 commits into from
Closed

Conversation

lukemckinstry
Copy link
Contributor

@lukemckinstry lukemckinstry commented Sep 23, 2024

Description

Bases zoom speed on the render rate of the user's browser.
Added fields in FrameRateMonitor to track the timestamps of the n most recent renders.
Then when calculating the zoom distance on each frame, adjust it based on the fps zoom speed to provide consistent experience for browsers with different speeds.
If the user wants faster or slower zoom, they can still adjust zoomFactor through the public api as before with the same effects.

Issue number and link

#12187

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @lukemckinstry!

✅ We can confirm we have a CLA on file for you.

@lukemckinstry lukemckinstry changed the title Base zoom speed on browser frames per second Base zoom speed on browser refresh rate Sep 23, 2024
@lukemckinstry
Copy link
Contributor Author

posted this for initial review/feedback/discussion @ggetz. I still need to add unit tests.

@javagl
Copy link
Contributor

javagl commented Sep 24, 2024

This looks somewhat similar to what I dumped into a forum thread a while ago. The goal there was also to have an "average frame rate over the last n frames".

In that forum thread, I put this into some FpsTracker class. The reasons may be obvious. The functionality is encapsulated in a class where the name says what the class does (/* An FpsTracker tracks the FPS */ 😁 ). It offers a getAverageFps function directly. It avoids that const timeStamps = object._scene.frameState?.timeStamps; (aka Train Wreck). And it avoids the manual computation (with magic constants like 10 and 30) inside the handleZoom function.

The purpose of that FpsTracker there was to plug it into some MsseUpdater, which was supposed the update the maximum screen space error based on the average FPS. (Fleshing this out into a real feature is still on my TODO list - I think that this could make a whole lot of sense...).

tl;dr : If there was "something" where a function like getAverageFps was offered, then this could probably be re-used in multiple places...

@lukemckinstry
Copy link
Contributor Author

I like your idea to create a FpsTracker class to make this more reusable. I will work on this change and update this PR when it is ready.

@ggetz
Copy link
Contributor

ggetz commented Sep 24, 2024

I like the idea of isolating the average FPS logic in one place. The update itself should probably still happen in Scene.js though, (as @lukemckinstry just implemented).

@ggetz ggetz self-assigned this Sep 25, 2024
@lukemckinstry
Copy link
Contributor Author

lukemckinstry commented Sep 26, 2024

We realized the existing FrameRateMonitor class provides close to the functionality this feature needs. We decided to make some small additions to it rather than create a new class (side notes: All the work up to this commit 331650c got re-written, so it may make sense to squash the early commits )

There may be questions as to why we dont get FPS from FrameRateMonitor.lastFramesPerSecond, this should be made clear by the doc updates included in this PR.

const fps = object.frameRateMonitor.averageFramesPerSecond;
// we set the ideal browser refresh rate to 30hz
if (fps) {
fpsMultiplier = 30 / fps;
Copy link
Contributor Author

@lukemckinstry lukemckinstry Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One open comment that I have not addressed: Including the target/ideal browser refresh rate as constant value (30 as in 30hz) in the code here. I am unsure if we should A) expose setting this in the API (but we already have zoomFactor in the public API) or B) set it in a more clear place, like a constants or system settings file.
I prefer option B, but I don't where that "good place to put it" would be

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

60 FPS is fairly standard, and I believe that is called out in the requestAnimatoinFrame documentation.

I don't think this value needs to be exposed, as it's more of a baseline against which zoomFactor is normalized. Feel free to call that out in the zoomFactor inline docs, but the description is already fairly arbitrary: "A multiplier for the speed at which the camera will zoom".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if FPS is defined, but the value is 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a check to make sure fps > 0, which should ensure we avoid division by 0

@lukemckinstry lukemckinstry force-pushed the framerate-over-time branch 3 times, most recently from d21b648 to 15cfa50 Compare September 27, 2024 18:04
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good @lukemckinstry! I have a few comments on code style, and a few thoughts on behavior.

packages/engine/Source/Core/FpsTracker.js Outdated Show resolved Hide resolved
.github/workflows/dev.yml Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
packages/engine/Source/Scene/FrameRateMonitor.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/FrameRateMonitor.js Outdated Show resolved Hide resolved
const fps = object.frameRateMonitor.averageFramesPerSecond;
// we set the ideal browser refresh rate to 30hz
if (fps) {
fpsMultiplier = 30 / fps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

60 FPS is fairly standard, and I believe that is called out in the requestAnimatoinFrame documentation.

I don't think this value needs to be exposed, as it's more of a baseline against which zoomFactor is normalized. Feel free to call that out in the zoomFactor inline docs, but the description is already fairly arbitrary: "A multiplier for the speed at which the camera will zoom".

const fps = object.frameRateMonitor.averageFramesPerSecond;
// we set the ideal browser refresh rate to 30hz
if (fps) {
fpsMultiplier = 30 / fps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if FPS is defined, but the value is 0?

fpsMultiplier = 30 / fps;
}

let zoomRate = zoomFactor * minDistance * fpsMultiplier;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working well when the frame rate is stable!

However I see some jerkiness when there's some big hiccups in frame rate, like when I'm running at 60FPS and then I move the camera and a lot of new data needs to be loaded in.

What do you link of linearly interpolating the fps multiplier? That way we should soften the impact of large FPS swings.

For example:

let fpsMultiplier = object._lastFpsMultiplier;
const fps = object.frameRateMonitor.averageFramesPerSecond;

...

// Interpolate 10% towards new multiplier
fpsMultiplier = CesiumMath.lerp(fpsMultiplier, 30 / fps, 0.1);

object._lastFpsMultiplier = fpsMultiplier;

Copy link
Contributor Author

@lukemckinstry lukemckinstry Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good point. I had not tested much while loading heavy data, and you are right that it substantially slows down the frame rate, and that makes this feature very jumpy.

I think linear interpolation is an improvement, but things are still pretty jumpy with lots of data.

Going back to the initial issue, the crux was to slow zoom speed on screens with fast refresh rates.

My initial solution tried to address slow and fast rates. I wonder if it makes sense to just address the case where we slow down the zoom on high refresh rate screens?

const fps = object.frameRateMonitor.averageFramesPerSecond;
...
fpsMultiplier = 60.0 / fps; // we could also use CesiumMath.lerp() here
fpsMultiplier = Math.min(fpsMultiplier, 1.0)
```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution. I wonder if we can set the min to something like 0.5 to still account for time when the FPS does dip a bit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be lacking some context, but would have thought that taking the average FPS was supposed to smooth out any hiccups in the FPS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to fill in context @javagl
The situation we are seeing when we test the branch on scenes with heavy data loading operations (like this sandcastle using the NY Buildings data https://sandcastle.cesium.com/?src=3D%20Tiles%20Feature%20Picking.html) is these heavy operations can make the render rate can slow down for a while (several seconds) or even indefinitely if the user continues operations that forces data reloading.

Low FPS leads to a high fpsMulitplier.

In the equation let zoomRate = zoomFactor * minDistance * fpsMultiplier minDistance is supposed to be small as we get close to the ellipsoid surface and large when far out in space to create intuitive UX. But high fpsMultiplier throws this off, yielding a high zoomRate when we are already zoomed in close, which explains the jerky/jumpy behavior we were referring to.

Back to the original complaint/issue. The problem is actually not when FPS is low, it is only when FPS is too fast. So the idea with fpsMultiplier = Math.min(fpsMultiplier, 1.0) is to only adjust the zoomRate downward towards what we expect from a 60 FPS screen when the screen has a much higher refresh rate (eg. 120hz).

@ggetz

This comment was marked as resolved.

@ggetz
Copy link
Contributor

ggetz commented Nov 8, 2024

@lukemckinstry I'm going to close this to keep things tidy. If you or anyone else wants to update and finish this out, please feel free to re-open!

@ggetz ggetz closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants